Skip to content

fix(modal): drop the PANELS borrow before closing drawer windows (#627) - #630

Merged
vibechoom merged 2 commits into
mainfrom
fix/modal-borrow-across-close-627
Jul 30, 2026
Merged

fix(modal): drop the PANELS borrow before closing drawer windows (#627)#630
vibechoom merged 2 commits into
mainfrom
fix/modal-borrow-across-close-627

Conversation

@vibechoom

Copy link
Copy Markdown
Contributor

Closes #627

Mechanism

close_all held panels.borrow_mut() (the drain() temporary) across every panel.window.close() call in its loop, while wire_retract_finish's child-revealed handler takes its own panels.borrow() of the same RefCell — if close() ever emitted that signal synchronously, the handler would run inside the still-active borrow_mut() and panic, which aborts the process (a panic unwinding through a glib callback). Fixed by draining PANELS into a local Vec first (dropping the borrow), then closing each owned panel outside the RefCell — strictly safer, no behavior change, matching the issue's proposed shape.

Verified the #624 reset_drawer_open_states() ordering is untouched: every panel is still dropped (via the Vec's consuming for loop) before reset_drawer_open_states() runs, exactly as before, so its documented "clear before prune" invariant holds. The plugin-panel clear and recompute_gates() afterward don't touch PANELS's borrow state, so they're unaffected by the reordering.

Blast-radius findings (step 3 — grepped modal.rs and overlays/, fixed only modal.rs per scope)

The identical shape (map.borrow_mut().drain() held across a .window.close() loop) also exists, unfixed, in:

  • trollshell/src/overlays/sidebar.rs::close_all (lines 594-621)
  • trollshell/src/overlays/frame.rs::close_all (lines 186-197)
  • trollshell/src/overlays/notifications.rs::close_all (lines 151-157)
  • trollshell/src/overlays/osd.rs::close_all (lines 192-209)

For each of these, I found no signal handler that currently takes a competing borrow()/borrow_mut() of that module's own map, so — like the modal.rs case before this fix — they appear dormant today, resting on the same overlays/sidebar.rs GTK-timing assumption. Not fixed here; flagging for a follow-up decision since other files may be claimed by other in-flight work.

A weaker, different-shaped instance: overlays/prompt.rs::close_prompt and two sites in overlays/consent.rs::close_all/request use if let Some(x) = cell.borrow_mut().take() { x.close(); } — Rust's temporary-lifetime-extension rule for if let scrutinees keeps the RefMut alive for the whole arm, so the borrow is technically still held across .close() even though the value itself was already moved out. No known reentrant borrower today either; noted for completeness rather than fixed.

Test

No test added. A hermetic reproduction isn't feasible — the hazard requires GTK to emit child-revealed synchronously from inside gtk_window_close, which can't be triggered on demand outside a live GTK main loop / display server, and the existing close_all_resets_drawer_open_state test already runs close_all against an empty PANELS (no display needed) so it doesn't exercise the borrow-timing question either way. Confirmed the existing test suite (cargo test -p trollshell, 233 tests) still passes unchanged.

Gates run

  • cargo clippy -p trollshell --all-targets -- -D warnings (RUSTC_WRAPPER unset) — clean
  • cargo test -p trollshell — 233 passed
  • nix fmt twice — 1 file changed then 0 changed

🤖 Generated with Claude Code

close_all held panels.borrow_mut() (the drain() temporary) across every
panel.window.close() call in its loop. wire_retract_finish's
child-revealed handler takes its own panels.borrow() on the same
RefCell; if a close() ever emitted that signal synchronously, the
handler would run inside the still-active borrow_mut() and panic — and
a panic unwinding through a glib callback aborts the process. Believed
dormant today only because of a GTK-internals assumption documented in
a different module (overlays/sidebar.rs's note on a closed window's
revealer never settling), which is a fragile thing to lean on.

Fixed per the issue's proposal: drain PANELS into a local Vec, letting
the borrow end before any window is closed, then close each owned
panel outside the RefCell. Verified the #624 reset_drawer_open_states()
ordering still holds — every panel is dropped (via the Vec's
consuming for-loop) before reset_drawer_open_states() runs, same as
before, so its "clear before prune" invariant is untouched. The
plugin-panel clear and recompute_gates afterward are unaffected; they
don't touch PANELS's borrow state.

Closes #627

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Review follow-up on #630, comment-only:

- close_all's doc cited overlays/sidebar.rs's settle-timer note as
  grounds for the hazard being dormant. That note is about a polling
  loop never terminating, not about whether close() emits
  notify::child-revealed synchronously — the actual open question. And
  overlays/notifications.rs documents a case where a destroy does fire
  a signal synchronously, so the citation oversold what's known.
  Reworded to "not verified either way" and dropped the citation; fixed
  the `overlays::sidebar.rs` malformed path/module mix in the process,
  and switched the `gtk::Window::close` intra-doc link to a plain
  backtick since `close` lives on the GtkWindowExt trait and may not
  resolve.
- Added a comment at wire_retract_finish's `None` branch: draining
  PANELS before any close() now means a synchronous re-entrant
  emission always lands here (the key is already gone), which is
  benign because close_all's own tail duplicates everything the `Some`
  arm would have done.

No code change; gates re-run (clippy clean, nix fmt 0 changed x2, the
14 modal:: tests unaffected).

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
@vibechoom
vibechoom merged commit aaba27a into main Jul 30, 2026
1 check passed
vibechoom added a commit that referenced this pull request Jul 30, 2026
…630/#634 (#636)

PR #634 (closes #613) makes two existing entries actively wrong rather than
merely stale, which is worse than a gap: a verifier following the old text
would read a successful self-heal as a regression. Corrects both, per #635:

- The #610 "restart NetworkManager" caveat no longer says the backend probe
  runs once at startup; it retries while inconclusive. The deferred re-probe
  gap now points at #633 (the genuinely-unsolved half — switching backends
  after one is already committed) instead of the closed #613.
- The #609 entry no longer expects an `error!` pointing at
  `systemctl --user restart trollshell` on a transient probe failure. The
  shipped retry policy is unbounded and self-heals into a `RECOVERED` line
  instead of giving up.

Also folds in the three PRs that merged alongside #634 since the doc's last
refresh (#628):

- #629 (closes #614): docs/plugin-env.md publishing to the Pages options-doc
  site, new "Documentation site (GitHub Pages)" section (deploy itself is
  outside `nix flake check` by design — flagged as a manual check).
- #630 (closes #627): modal::close_all's borrow-timing fix, added to Shell
  chrome as a regression-only check — explicitly states that no observable
  change is the pass condition.
- #634 itself: new Wi-Fi entry covering the INCONCLUSIVE/RECOVERED journal
  pair and the incidental startup-freeze fix.
- #628: docs-only refresh of this file, noted in the closing section.

Coverage line bumped to #458 through #634. Documentation-only change.

Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
vibechoom added a commit that referenced this pull request Jul 30, 2026
…e-enter them (#631) (#644)

* fix(overlays): release RefCell borrows before the GTK call that can re-enter them (#631)

A RefCell borrow held across a GTK call (close()/destroy(), or a
futures-signals set_neq() that wakes a subscriber) is a latent
reentrancy hazard: if the emission is ever synchronous and a handler
borrows the same cell, it panics, and a panic unwinding through a
glib callback aborts the process. PR #630 fixed the first instance in
modal::close_all; this sweeps the rest.

Form A (borrow_mut().drain() held across the loop's teardown call) in
sidebar.rs, frame.rs, notifications.rs, osd.rs: switched to
RefCell::take(), which moves the whole map out (leaving Default) and
releases the borrow inside the call — no intermediate Vec, and keeps
keys where the loop needs them (sidebar.rs).

Form B (if let Some(x) = cell.borrow_mut().take() with a GTK call in
the then-block) in prompt.rs's close_prompt and consent.rs's close_all
plus its supersede-in-request site: bind the taken value first, then
act on it outside the borrow. Rust 2024 only changed when an if let
scrutinee's temporary drops relative to an else block; the then-block
lifetime this hazard depends on is unaffected.

Two more in modal.rs the original survey missed:
- reset_drawer_open_states held DRAWER_OPEN borrowed across set_neq()
  for every entry; now collects the Mutable handles first and calls
  set_neq() on them with no borrow held.
- install's panels.borrow_mut().insert(...) dropped the replaced
  Option<ModalPanel> as a same-statement temporary of the borrow_mut()
  RefMut — statement temporaries drop in reverse creation order, so
  the old panel dropped before the RefMut, i.e. still inside the
  borrow. Wrapped in drop(PANELS.with(...)) so it drops after with()
  returns.

prompt.rs's abort_subscription has the same take()-shape but aborts a
tokio AbortHandle, not GTK — deliberately left alone, as the issue
calls out.

Reachability is unverified either way, not dormant: sidebar.rs's
settle-timer note is about a polling loop, not signal synchronicity,
and notifications.rs documents a case where a signal (connect_unmap)
does fire synchronously from inside a teardown. This removes the
dependency on GTK's emission timing regardless of which way that
resolves.

No hermetic test: reproducing the hazard needs GTK to emit a signal
synchronously from inside destroy()/close() against a real display,
and trollshell has no display-test harness to fake that.

Also grepped for the same two shapes outside this issue's site list
and found more, unfixed — filed as #643 rather than fixed here.

Closes #631

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

* fix(modal): correct a stale doc claim + add the one hermetically-testable reentrancy site (#631 follow-up)

Adversarial review of #644 confirmed reset_drawer_open_states is
correct, but flagged two gaps:

1. The doc comment on close_all/reset_drawer_open_states still said
   "the PANELS drain above has already dropped every
   ModalPanel::open_state, so by the time we run, this map holds the
   last Mutable for each key" — true of the pre-#631 shape, false now.
   Cloning into `states` before set_neq bumps senders 1->2, so the
   map's copy is never the last survivor: retain's drop of it is an
   ordinary decrement, and the actual last-sender drop happens only
   when `states` goes out of scope at the end of the function, after
   both the reset and the prune, with no DRAWER_OPEN borrow held.
   Rewrote the comment to say that, and to note explicitly that this
   is the safe direction relative to the pre-#631 shape (whose
   last-sender drop happened at retain, inside the borrow).

2. reset_drawer_open_states has no GTK in it at all — its reentrancy
   trigger is plain futures-signals Waker::wake(), invoked
   synchronously from Mutable::set_neq's notify. That's testable with
   nothing but std::task: a std::task::Wake impl (not a raw
   RawWaker/RawWakerVTable — the workspace forbids unsafe_code
   project-wide, and Wake's Arc<W> -> Waker conversion is the safe,
   stable route) whose wake() re-enters DRAWER_OPEN, registered on a
   signal for one key, then reset_drawer_open_states run under
   catch_unwind.

   Falsified against the pre-#631 shape before restoring: it doesn't
   just fail the assert. The BorrowMutError panics through Mutable's
   internal Mutex mid-notify, poisoning it; catch_unwind catches that
   first panic, but the poisoned Mutable is still sitting in
   DRAWER_OPEN, and the worker thread's later TLS teardown re-panics
   dropping it -- from inside a destructor, where an escaping panic is
   fatal. `cargo test -p trollshell modal::` aborts the whole binary
   (SIGABRT), not just one test failing. Restored the fix: 234 tests
   pass (233 + this one), clippy clean, nix fmt 0 changes.

This is the one site in the #631 sweep whose regression would
otherwise be silent -- every other test in the module would keep
passing while this function quietly reopened the reentrancy hazard.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

---------

Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
vibechoom added a commit that referenced this pull request Jul 31, 2026
…urst falsified (#672)

Closes #660

Bring docs/live-verify.md current across the ten PRs that merged after the
last refresh (#634): #637, #639, #644, #645, #662, #663, #664, #666, #668,
plus #642 (coverage bump only, per the issue's own conclusion — its default
station-id fix needs no live-verify caveat since write_default_config only
touches an absent places.toml).

Corrected the #630 entry: it told a verifier to expect modal::close_all
tearing down with window.close(); #637 changed that call to destroy() after
#630 had already merged, so the entry named a call the code no longer makes
(exactly the issue's finding). Also added a new #645 entry to the Network
panel section for the networkd startup-refresh retry, since that path used to
latch "no link manager has answered yet" forever on a transient first-refresh
failure (curable only by a shell restart) and now self-heals — swept the rest
of the doc for the same class of falsified expectation (inert-until-restart)
and found nothing else affected; the wifi-side #609/#634 entries describe a
different code path and are unaffected.

New entries added:
- #637/#639 — the destroy()-not-close() sweep across the five trollshell
  overlays and the three remaining hytte-ui sites (bar.rs, popup.rs),
  including #637's WeakRef/toplevels()-count kanshi hot-plug gesture.
- #644/#663 — the RefCell-borrow-across-GTK-call sweep (31 sites total
  across overlays/modal.rs and the rest of the UI, incl. an 8-panel-wide
  reactive_list.rs helper and the control-center). Both documented with the
  same "nothing changes" honest pass condition #630 already used.
- #664 — mpris Auto chip fix + bare-string artist widening. Needs two live
  MPRIS players and a shell restart for the new CSS class, both called out.
- #666 — new top-level "Claude bridge" section for the keyless loopback
  shim, nobody has run this service live yet. Carries the PR's flagged
  riskiest assumption (hive-claude's SessionNotFound classification) with
  its journal tell.
- #668 — new top-level "D-Bus name ownership" section. Documents that
  recovery going from ~250ms to up to 5 minutes after a squatter exits is
  the expected cost of the fix, not a regression to file.

Left alone: #642 (no doc change needed beyond the coverage line, matching
the issue's own recommendation after checking places.toml's write-once
behavior); #637's abort_subscription/prompt.rs exclusions and #644/#663's
"deliberately not touched" sites (no live-visible behavior, nothing to
verify).

Gates: markdown-only diff (git diff --stat: 1 file, docs/live-verify.md).
nix fmt run twice (1 file reformatted first pass — a prose reflow — 0
changed second pass). Clippy/tests deliberately skipped: nix/package.nix's
crane filter excludes markdown, so the packaged derivation is unaffected and
a nix build would burn a full workspace recompile for zero signal (the same
crane-filter trap #662 already documented). nix build was not run, per
instructions.

Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
vibechoom added a commit that referenced this pull request Jul 31, 2026
…uster, install inserts, shared borrows (#643) (#673)

The overlay half of #643, sequenced behind #644/#663 which owned these
files. 22 sites, all four spellings of the corrected definition (*any*
borrow held across a GTK call, not just `borrow_mut()`).

notifications.rs `apply_emission` is the headline: two named `RefMut`s
plus the overflow slot held live across `vbox.remove`, `replace_card`/
`mount_card`, the overflow `remove`/`append` and the closing
`set_visible`, whose argument was itself an `overflow_card.borrow()`
temporary. It is the one site in the cluster whose emission the tree
*documents* as synchronous (`connect_unmap` → `resume_expiry`, the
mechanism #593's design rests on); it survives today only because that
handler touches the expiry bookkeeping rather than `card_map`. Converted
with take-and-restore, the `panels/stats.rs` idiom from #663.

`TOAST_WINDOWS` and `PANELS` (modal) now store `Rc<_>` so `route_emission`
and modal's nine entry points can clone a handle out instead of holding a
shared borrow across `show_panel` / `set_stack_page` / `set_reveal_child`
/ `on_page_show` — mirroring `overlays::osd`'s existing `Rc<OsdView>`.
`modal::live_panel`/`live_panels` are the snapshot helpers.

The four `install` insert-drop sites (sidebar/frame/osd/consent) get the
tail-expression + `drop(...)` shape #644 annotated at `modal::install`.
Their comments state the mechanism precisely per the #663 review's
correction: a refcount decrement, not a widget teardown. consent's is the
weakest of the four — a `GdkMonitor` unref emits nothing — and says so.

Also `tasks.rs::refresh_summary` (a `match *self.mode.borrow()` holding
across every arm's `set_visible`/`set_text`, which #663 missed inside a
range its body certified clean), `tasks.rs::do_create`'s getter-ordering
folklore, and four sites the closing re-grep turned up in
`widgets/{workspaces,tray,window_list}.rs`.

No new tests: `trollshell` is a binary crate with no `system-tests`/xvfb
bucket, and every site needs a live GTK widget tree to reach. The
`Waker`-based harness doesn't apply — the two sites here with a `Mutable`
wake inside the borrow still need a populated `PANELS`, i.e. GTK windows.

Closes #643. Refs #663, #644, #631, #630, #627, #593.

Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(modal): close_all holds PANELS.borrow_mut() across window.close(), one synchronous child-revealed away from an abort

1 participant